-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(aci): Ensure DetectorGroup for recurring Groups #103419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103419 +/- ##
===========================================
- Coverage 80.67% 80.67% -0.01%
===========================================
Files 9243 9243
Lines 394826 394936 +110
Branches 25152 25152
===========================================
+ Hits 318538 318615 +77
- Misses 75841 75874 +33
Partials 447 447 |
mifu67
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're only touching error issues here—ensure_association_with_detector is never called with a detector ID. Did you want to handle other grouptypes in a separate PR?
In any case, I think that save_issue_occurrence is a good place to handle healing the relationship for metric issues. We can get the detector ID using occurrence_data.evidence_data. Let me know too if you want me to handle that.
I was initially just targeting errors here, since for metric issues there was already a migration PR out, but I intentionally left the helper method more general to allow us to extend it to other issue types. So, happy to do that here. |
mifu67
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me from the metric issue perspective. Feel free to merge, unless you want a review from the errors perspective too.
We want DetectorGroups for all Groups that can associate with a Detector.
We already have path to add it for all new groups, but backfilling existing groups is a little tricky,
and by doing it incrementally be recurrence here, we can make DetectorGroup reliable for Detector lookup
for essentially any Group the workflow engine is asked to process.
We may still need a backfill for completeness if we can't wait until the Group retention window for completeness.